#159 Code sanitisation (Updated)#240
#159 Code sanitisation (Updated)#240FedericoTartarini merged 79 commits intoCenterForTheBuiltEnvironment:developmentfrom
Conversation
…long with examples of code modifications.
…nd template_graphs.py
…rts_data_explorer.py
…y-chart.py and summary.py
… and natural_ventilation.py.
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
pages/lib/template_graphs.py (2)
410-415: Bug: margin becomes None due to dict.update() return valuedict.update() returns None; Plotly receives margin=None.
- fig.update_layout( - template=template, - title=title, - margin=tight_margins.copy().update({"t": 55}), - yaxis_nticks=13, - ) + fig.update_layout( + template=template, + title=title, + margin={**tight_margins, "t": 55}, + yaxis_nticks=13, + )
696-703: Same bug: margin None via dict.update()Repeat of the margin issue in thermal_stress_stacked_barchart.
- fig.update_layout( + fig.update_layout( legend=dict(orientation="h", yanchor="bottom", y=1.05, xanchor="right", x=1), barmode="stack", dragmode=False, title=title, - margin=tight_margins.copy().update({"t": 55}), + margin={**tight_margins, "t": 55}, )pages/select.py (3)
143-166: Replace hard-coded ctx.prop_id strings with ElementIds to stay consistent with centralizationUse constants instead of literal IDs and guard the upload button trigger to avoid unnecessary work.
- if ctx.triggered[0][ColNames.PROP_ID] == "modal-yes-button.n_clicks": + if ctx.triggered[0][ColNames.PROP_ID] == f"{ElementIds.MODAL_YES_BUTTON}.n_clicks": @@ - elif ( - ctx.triggered[0][ColNames.PROP_ID] == "upload-data.contents" - and list_of_contents is not None - ): + elif ctx.triggered[0][ColNames.PROP_ID] == f"{ElementIds.UPLOAD_DATA_BUTTON}.n_clicks": + raise PreventUpdate + elif ( + ctx.triggered[0][ColNames.PROP_ID] == f"{ElementIds.UPLOAD_DATA}.contents" + and list_of_contents is not None + ):
172-188: Harden EPW file-type detection; current "epw in name" is brittleUse a strict, case-insensitive extension check; also fix the misleading comment.
- if "epw" in list_of_names[0]: - # Assume that the user uploaded a CSV file + name = (list_of_names[0] or "").lower() + if name.endswith(".epw"): + # User uploaded an EPW file
195-203: Return the correct alert on parse errorsParsing failures for .epw should use the "invalid_format" message, not "wrong_extension".
- except (ValueError, IndexError, KeyError) as e: + except (ValueError, IndexError, KeyError) as e: print(f"Error parsing EPW file: {e}") return ( None, None, True, - messages_alert["wrong_extension"], + messages_alert["invalid_format"], "warning", )
♻️ Duplicate comments (8)
docs/contributing/contributing.md (2)
21-24: Fix clone URL placeholder (space breaks copy/paste).Replace the placeholder with a safe, copyable template.
-git clone https://github.com/Your Account name/clima.git +git clone https://github.com/<your-account-name>/clima.git
54-61: Fix typo and clarify instructions for checking out development.“checktout” → “checkout”; rephrase for clarity.
-Pull the development branch first, and if the terminal does not notice you that you should try the second command. +Pull the development branch first. If that fails, try the second command. @@ -git checktout development +git checkout developmentpages/lib/charts_sun.py (2)
77-101: monthly_solar (Diffuse): apply same customdata fix as above.- customdata=epw_df.loc[ - epw_df[ColNames.MONTH] == i + 1, ColNames.MONTH_NAMES - ], + customdata=[month_lst[i]] * int( + dif_h_rad_month_ave.loc[ + dif_h_rad_month_ave[ColNames.MONTH] == i + 1 + ].shape[0] + ),
45-66: monthly_solar: customdata length mismatch; broadcast month label to 24 points.
g_h_rad_month_avehas 24 rows per month, butepw_df[...]yields 672–744 rows. Plotly requires equal lengths.- customdata=epw_df.loc[ - epw_df[ColNames.MONTH] == i + 1, ColNames.MONTH_NAMES - ], + customdata=[month_lst[i]] * int( + g_h_rad_month_ave.loc[ + g_h_rad_month_ave[ColNames.MONTH] == i + 1 + ].shape[0] + ),pages/lib/extract_df.py (1)
164-165: Bug: FAKE_YEAR column is assigned the literal "year" stringThis writes the string constant instead of copying the dataframe values.
- epw_df[ColNames.FAKE_YEAR] = ColNames.YEAR + epw_df[ColNames.FAKE_YEAR] = epw_df[ColNames.YEAR]pages/psy-chart.py (1)
176-176: Trailing space after ElementIds.PSY_VAR_DROPDOWNRemove the stray space to avoid accidental ID/key mismatches.
- id=ElementIds.PSY_VAR_DROPDOWN , + id=ElementIds.PSY_VAR_DROPDOWN,- State(ElementIds.PSY_VAR_DROPDOWN , "value"), + State(ElementIds.PSY_VAR_DROPDOWN, "value"),Also applies to: 252-252
pages/wind.py (2)
405-417: Wind direction callback should also react to DF updatesWithout the DF timestamp Input, graph won’t refresh on data changes. This was flagged before.
-@callback( - Output(ElementIds.WIND_DIRECTION, "children"), - # General - [ - Input(ElementIds.ID_WIND_GLOBAL_LOCAL_RADIO_INPUT, "value"), - ], +@callback( + Output(ElementIds.WIND_DIRECTION, "children"), + # General + [ + Input(ElementIds.ID_WIND_DF_STORE, "modified_timestamp"), + Input(ElementIds.ID_WIND_GLOBAL_LOCAL_RADIO_INPUT, "value"), + ], @@ -def update_tab_wind_direction(global_local, df, meta, si_ip): +def update_tab_wind_direction(_, global_local, df, meta, si_ip):
631-633: Bug: Noon selection uses morning time rangeUse noon_times for noon_df.
- noon_df = df.loc[ - (df[ColNames.HOUR] >= morning_times[0]) & (df[ColNames.HOUR] <= morning_times[1]) - ] + noon_df = df.loc[ + (df[ColNames.HOUR] >= noon_times[0]) & (df[ColNames.HOUR] <= noon_times[1]) + ]
🧹 Nitpick comments (20)
docs/contributing/contributing.md (4)
48-52: Replace hard tabs with spaces in branch list.Tabs violate MD010 and render inconsistently.
-> * main - remotes/origin/HEAD -> origin/main - remotes/origin/development - remotes/origin/main +> * main + remotes/origin/HEAD -> origin/main + remotes/origin/development + remotes/origin/main
64-66: Use angle-bracket placeholders and avoid parentheses in commands.-git checkout -b (your branch name) +git checkout -b <your-branch-name>
68-72: Tighten wording and fix placeholder in push command.-Finally update and push to your repository branch if you modify the files. +Finally, push your branch to your fork after committing changes. @@ -git push origin (your branch name) +git push origin <your-branch-name>
99-119: Remove duplicated Ruff section under “Install Black”.This block repeats lines 80–98 verbatim and adds noise.
-Install Black: - -We use ruff to enforce the code style and code formatting. You can run it with: - -```bash -pipenv run ruff check . -pipenv run ruff format . -``` - -To ensure that the code is formatted correctly, we use a pre-commit hook that runs Ruff before every commit. -Run the following once to enable hooks in your local repo: - -```bash -pipenv run pre-commit install -# optional: run on all files -pipenv run pre-commit run --all-files -``` - -Hence, you will need to make sure that the code is formatted correctly before committing your changes; otherwise, the commit will fail. -More information about pre-commit hooks can be found [here](https://pre-commit.com/). - +Install Black:pages/lib/global_column_names.py (3)
4-12: Consider separating DataFrame column names from metadata/UI keys.ColNames mixes true columns (e.g., YEAR, DBT) with generic dict keys (NAME, UNIT, RANGE, COLOR). This coupling leaks UI concerns into data schema and invites misuse.
Proposed direction:
- Keep ColNames for DataFrame columns only.
- Introduce a separate Keys (or MetaKeys) Enum for {'name','unit','range','color','conversion_function','prop_id',...}.
I can draft a small patch once you confirm the split is acceptable.
86-109: Minor consistency: group “calculation” vs. “meta” keys.Entries like TWENTY_FOUR_HOUR, TIMES, TO_IMAGE_BUTTON_OPTIONS, PROP_ID are not columns. If keeping a single Enum, at least add a comment section header to avoid accidental use in pandas ops.
61-76: Naming consistency for UTCI members.UTCI_* members mix camelCase in values (e.g., utci_Sun_noWind). If feasible, adopt a consistent snake_case value convention across new data being generated to reduce future mapping shims.
pages/lib/charts_summary.py (1)
7-12: Meta access via ColNames is correct; consider tolerant parsing for time zone.Casting meta[ColNames.TIME_ZONE] to float will fail if the value is like "Etc/GMT-8". If upstream may provide TZ names, add a try/except with a default.
- time_zone = float(meta[ColNames.TIME_ZONE]) + try: + time_zone = float(meta[ColNames.TIME_ZONE]) + except (TypeError, ValueError): + time_zone = 0.0 # fallback; display unaffectedpages/lib/charts_sun.py (2)
121-149: Redundant computation and duplicate filter.
timesis computed then unused;solpos = df.loc[df[APPARENT_ELEVATION] > 0]appears twice.- tz = "UTC" - times = pd.date_range( - "2019-01-01 00:00:00", "2020-01-01", inclusive="left", freq="h", tz=tz - ) - delta = timedelta(days=0, hours=time_zone - 1, minutes=0) - times = times - delta - solpos = df.loc[df[ColNames.APPARENT_ELEVATION] > 0, :] + tz = "UTC" + delta = timedelta(days=0, hours=time_zone - 1, minutes=0) + solpos = df.loc[df[ColNames.APPARENT_ELEVATION] > 0, :]
428-459: Minor: month list uses mixed zero-padded dates.
"2019-4-21"/"2019-5-21"parse fine but are inconsistent with zero-padded forms used elsewhere. Optional cleanup.- for date in pd.to_datetime(["2019-01-21", "2019-02-21", "2019-4-21", "2019-5-21"]): + for date in pd.to_datetime(["2019-01-21", "2019-02-21", "2019-04-21", "2019-05-21"]):pages/lib/extract_df.py (3)
253-254: Prefer ColNames over attribute access for index alignmentUsing epw_df.times breaks the intent of centralizing column names and can fail if the column is renamed.
- mrt_df = pd.DataFrame.from_records(mrt) - mrt_df = mrt_df.set_index(epw_df.times) + mrt_df = pd.DataFrame.from_records(mrt) + mrt_df = mrt_df.set_index(epw_df[ColNames.TIMES])
265-267: Simplify “no-wind” speed assignmentMasking with >= 0 replaces all values; set the column once for clarity and speed.
- epw_df[ColNames.WIND_SPEED_UTCI_0] = epw_df[ColNames.WIND_SPEED_UTCI].mask( - epw_df[ColNames.WIND_SPEED_UTCI] >= 0, 0.5 - ) + epw_df[ColNames.WIND_SPEED_UTCI_0] = 0.5
381-388: Avoid double JSON parsing and use the already loaded dictReduces overhead and is clearer.
- mapping_dict = json.loads(mapping_json) - for key in json.loads(mapping_json): + mapping_dict = json.loads(mapping_json) + for key in mapping_dict: if ColNames.CONVERSION_FUNCTION in mapping_dict[key]: conversion_function_name = mapping_dict[key][ColNames.CONVERSION_FUNCTION] if conversion_function_name is not None: conversion_function = globals()[conversion_function_name] conversion_function(df, key) return json.dumps(mapping_dict)pages/psy-chart.py (1)
281-283: Confirm intent: filtering sets ALL columns to NonePassing df.columns to filter_df_by_month_and_hour nulls every column for out-of-range rows. This is heavier than masking just the plotted variable(s) and can impact downstream computations.
If the goal is to filter only plotted/needed fields, pass a list of those columns instead of df.columns.
pages/select.py (4)
217-217: Use ElementIds for State instead of the raw stringKeep identifiers centralized.
- [State(ElementIds.ID_SELECT_URL_STORE, "data"), State("lines-store", "data")], + [State(ElementIds.ID_SELECT_URL_STORE, "data"), State(ElementIds.LINES_STORE, "data")],
69-72: Avoid placeholder Div with the same ID that later becomes a GraphUsing a Div with id=TAB_ONE_MAP while callbacks read clickData from that id is fragile. Initialize a Graph placeholder instead to prevent property mismatches.
- dmc.Skeleton( - visible=False, - id=ElementIds.SKELETON_GRAPH_CONTAINER, - height=500, - children=html.Div(id=ElementIds.TAB_ONE_MAP), - ), + dmc.Skeleton( + visible=False, + id=ElementIds.SKELETON_GRAPH_CONTAINER, + height=500, + children=dcc.Graph(id=ElementIds.TAB_ONE_MAP, figure={"data": [], "layout": {}}), + ),Also applies to: 322-377
355-355: Pass the column name, not a Series, to hover_nameThis keeps Plotly Express semantics and aligns with constant usage.
- hover_name=df_one_building[ColNames.NAME], + hover_name=ColNames.NAME,
278-279: Guard against missing meta keys to avoid KeyErrorIf meta is None or missing keys, this will error. Consider safe access.
- "Current Location: " + meta[ColNames.CITY] + ", " + meta[ColNames.COUNTRY], + f"Current Location: {meta.get(ColNames.CITY, 'N/A')}, {meta.get(ColNames.COUNTRY, 'N/A')}",pages/outdoor.py (1)
315-325: Replace if/elif chain with a mapping for image selectionSimpler, clearer, and easier to extend.
-def change_image_based_on_selection(value): - if value == "utci_Sun_Wind": - source = "./assets/img/sun_and_wind.png" - elif value == "utci_Sun_noWind": - source = "./assets/img/sun_no_wind.png" - elif value == "utci_noSun_Wind": - source = "./assets/img/no_sun_and_wind.png" - else: - source = "./assets/img/no_sun_no_wind.png" - - return html.Img(src=source, height=50) +def change_image_based_on_selection(value): + mapping = { + "utci_Sun_Wind": "./assets/img/sun_and_wind.png", + "utci_Sun_noWind": "./assets/img/sun_no_wind.png", + "utci_noSun_Wind": "./assets/img/no_sun_and_wind.png", + "utci_noSun_noWind": "./assets/img/no_sun_no_wind.png", + } + return html.Img(src=mapping.get(value, mapping["utci_Sun_Wind"]), height=50)pages/wind.py (1)
514-514: Use ColNames in query strings for calm wind detectionKeeps naming centralized and avoids accidental drift.
- query_calm_wind = "wind_speed == 0" + query_calm_wind = f"{ColNames.WIND_SPEED} == 0"Also applies to: 624-624
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (18)
docs/contributing/contributing.md(3 hunks)docs/documentation/tabs-explained/outdoor-comfort/utci-explained.md(1 hunks)docs/documentation/tabs-explained/sun-and-cloud/global-and-diffuse-horizontal-solar-radiation/README.md(1 hunks)pages/lib/charts_data_explorer.py(6 hunks)pages/lib/charts_summary.py(1 hunks)pages/lib/charts_sun.py(16 hunks)pages/lib/extract_df.py(8 hunks)pages/lib/global_column_names.py(1 hunks)pages/lib/global_scheme.py(2 hunks)pages/lib/layout.py(2 hunks)pages/lib/template_graphs.py(25 hunks)pages/lib/utils.py(7 hunks)pages/natural_ventilation.py(25 hunks)pages/outdoor.py(18 hunks)pages/psy-chart.py(20 hunks)pages/select.py(15 hunks)pages/summary.py(20 hunks)pages/wind.py(27 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/documentation/tabs-explained/sun-and-cloud/global-and-diffuse-horizontal-solar-radiation/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/documentation/tabs-explained/outdoor-comfort/utci-explained.md
- pages/lib/layout.py
- pages/lib/charts_data_explorer.py
- pages/natural_ventilation.py
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/contributing/contributing.md
49-49: Hard tabs
Column: 3
(MD010, no-hard-tabs)
50-50: Hard tabs
Column: 3
(MD010, no-hard-tabs)
51-51: Hard tabs
Column: 3
(MD010, no-hard-tabs)
🪛 LanguageTool
docs/contributing/contributing.md
[grammar] ~133-~133: There might be a mistake here.
Context: ...ease make sure: - All tests should pass. - You have installed project dependencies:...
(QB_NEW_EN)
[grammar] ~164-~164: There might be a mistake here.
Context: ...iew and CI; merge and release every time, - Develop: Continuous Integration branch for dail...
(QB_NEW_EN)
[grammar] ~166-~166: There might be a mistake here.
Context: ...o merge in after completing the feature. - Fix/*: defect repair branch, the same process...
(QB_NEW_EN)
[grammar] ~167-~167: There might be a mistake here.
Context: ...pair branch, the same process as feature - Release/*: release preparation branch for freezin...
(QB_NEW_EN)
[grammar] ~168-~168: There might be a mistake here.
Context: ...entation, doing regressions and tagging. - docs/*, chore/*, refactor/*, test/*: doc...
(QB_NEW_EN)
[grammar] ~169-~169: There might be a mistake here.
Context: ...ellaneous, refactor, test type branches. - Style: style modification (does not affect th...
(QB_NEW_EN)
[grammar] ~171-~171: There might be a mistake here.
Context: ...xisting code to improve maintainability. - Test: Add or modify tests: add unit tests, i...
(QB_NEW_EN)
[grammar] ~174-~174: There might be a mistake here.
Context: ...de execution efficiency or memory usage. - Ci: CI Configuration Related: Changing Con...
(QB_NEW_EN)
[grammar] ~175-~175: There might be a mistake here.
Context: ... memory usage. - Ci: CI Configuration Related: Changing Continuous Integration Configu...
(QB_NEW_EN)
[grammar] ~175-~175: There might be a mistake here.
Context: ...or Github Actions, Travis, Jenkins, etc. - Build: build system related: modify build scr...
(QB_NEW_EN)
[grammar] ~177-~177: There might be a mistake here.
Context: ...llback Commit: Undoing a Previous Commit - Security: Security fixes, fixing security vulner...
(QB_NEW_EN)
[grammar] ~178-~178: There might be a mistake here.
Context: ...pdating dependencies to prevent attacks. - Deps: Dependency Management: Dependency Mana...
(QB_NEW_EN)
[grammar] ~179-~179: There might be a mistake here.
Context: ...ating, and removing dependency libraries - Infra: Infrastructure related: changes to dev...
(QB_NEW_EN)
[grammar] ~182-~182: There might be a mistake here.
Context: ...ations, etc. ## Pull Request Regulation Time to submit PR: - User requirement...
(QB_NEW_EN)
[grammar] ~185-~185: There might be a mistake here.
Context: ...in Issue and consensus has been reached. - Changes have been minimised (small steps...
(QB_NEW_EN)
🔇 Additional comments (10)
pages/lib/global_scheme.py (3)
2-2: LGTM: centralized key usage via ColNames.NAME.Importing ColNames and indexing with ColNames.NAME removes magic strings and improves consistency.
903-906: Comprehension uses trusted mapping keys; add guard only if inputs are dynamic.If variable lists are static (as here), this is fine. If they become user-driven, wrap with
.get(..., {})to avoid KeyError.
907-920: LGTM: consistent replacement of ["name"] with ColNames.NAME across maps.No functional change; improves maintainability.
pages/lib/charts_sun.py (1)
129-133: Key indirection via ColNames works; verify all mapping_dictionary entries use these keys.Especially for older entries, ensure keys “name”, “unit”, “range”, “color” all exist to avoid KeyError.
pages/lib/utils.py (1)
261-269: LGTM: invert flags centralized via ColNamesUsing ColNames.INVERT for both month and hour improves consistency with the new constants.
pages/summary.py (1)
260-277: Defensive calc for solar radiation: LGTMGood guard against division by zero and consistent unit handling with ColNames.
pages/select.py (1)
333-336: LGTM: Good use of ColNames for features/coordinates/lat/lonConsistent with the new constants approach.
pages/outdoor.py (1)
239-243: LGTM: Good migration to ColNames for UTCI category columnsConsistent and future-proof.
pages/wind.py (2)
381-401: LGTM: Output now targets ElementIds.WIND_SPEEDFixes the earlier mismatch with a non-existent layout id.
55-63: Confirm hour indexing consistency (1–24 vs 0–24)Sliders here use 1–24 (wind tab) while outdoor uses 0–24. Ensure alignment with DF hour encoding.
Also applies to: 613-617
Refactor multiple files to enhance code clarity by adjusting line breaks, spacing, and indentation. This includes consistent formatting in function calls and variable assignments across various modules.
Update the label text from "Yearly_chart" to "Yearly Chart" for better readability and consistency in the user interface.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Introduce a new constant for the seasonal wind rose document ID in global_element_ids.py and update the wind.py to utilize this ID for the corresponding button.
|
@LeoLuosifen Thank you for working on this, nice work. However please note the following issues:
Please review all these issues and make sure that error looks like the above are all fixed so i can do a final review. |
…atural_ventilation.py, psy-chart.py, summary.py, sun.py, wind.py, t-rh.py and outdoor.py
- add global tab names python file to store tab name value - replaced the tab_name via using global_tab_names.py - removed unused import: `enum.Enum` - formatted the code style via running ruff and pre-commit
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
pages/lib/extract_df.py (1)
195-223: Unify change_to_float with ColNames and guard missing columns.Use constants and intersect with existing columns to avoid KeyErrors for short files.
- change_to_float = [ - "DBT","DPT","RH","p_atm","extr_hor_rad","hor_ir_rad","glob_hor_rad", - "dir_nor_rad","dif_hor_rad","glob_hor_ill","dir_nor_ill","dif_hor_ill", - "Zlumi","wind_dir","wind_speed","tot_sky_cover","Oskycover","Vis", - "Cheight","PWobs","PWcodes","Pwater","AsolOptD","SnowD","DaySSnow", - ] - epw_df[change_to_float] = epw_df[change_to_float].astype(float) + change_to_float = [ + ColNames.DBT, ColNames.DPT, ColNames.RH, ColNames.P_ATM, + ColNames.EXTR_HOR_RAD, ColNames.HOR_IR_RAD, ColNames.GLOB_HOR_RAD, + ColNames.DIR_NOR_RAD, ColNames.DIF_HOR_RAD, ColNames.GLOB_HOR_ILL, + ColNames.DIR_NOR_ILL, ColNames.DIF_HOR_ILL, ColNames.ZLUMI, + ColNames.WIND_DIR, ColNames.WIND_SPEED, ColNames.TOT_SKY_COVER, + ColNames.OSKY_COVER, ColNames.VIS, ColNames.CHEIGHT, ColNames.PWOBS, + ColNames.PWCODES, ColNames.PWATER, ColNames.ASOL_OPTD, ColNames.SNOWD, + ColNames.DAYSSNOW, + ] + present = [c for c in change_to_float if c in epw_df.columns] + epw_df[present] = epw_df[present].astype(float)pages/lib/global_element_ids.py (1)
1-226: Centralize all remaining literal component IDs
- main.py (ln 17):
dcc.Location(id="url", …)- main.py (ln 19):
html.Div(id="page-content", …)- pages/select.py (ln 327):
Input("url", "pathname")- pages/summary.py (ln 194–195, commented):
Output('input-hdd-set-point', …), Input('si-ip-radio-input', …)Replace each literal with the appropriate
ElementIdsconstant (adding new entries forURLandPAGE_CONTENTif they don’t yet exist).pages/lib/layout.py (2)
14-25: Add rel="noopener noreferrer" to external links.Prevents reverse tabnabbing with target="_blank".
- html.A( + html.A( "quick user survey", href="https://forms.gle/k289zP3R92jdu14M7", className="alert-link", - target="_blank", + target="_blank", + rel="noopener noreferrer", ),Also applies to: 27-39
82-124: Add rel to all footer anchors opened in new tabs.- dmc.Anchor( + dmc.Anchor( "Version: 0.9.0", href="https://center-for-the-built-environment.gitbook.io/clima/version/changelog", underline=True, c="white", - target="_blank", + target="_blank", + rel="noopener noreferrer", ), - dmc.Anchor( + dmc.Anchor( "Contributors", href="https://cbe-berkeley.gitbook.io/clima/#contributions", underline=True, c="white", - target="_blank", + target="_blank", + rel="noopener noreferrer", ), - dmc.Anchor( + dmc.Anchor( "Report issues on GitHub", href="https://github.com/CenterForTheBuiltEnvironment/clima/issues", underline=True, c="white", - target="_blank", + target="_blank", + rel="noopener noreferrer", ), - dmc.Anchor( + dmc.Anchor( "Contact us", href="https://github.com/CenterForTheBuiltEnvironment/clima/discussions", underline=True, c="white", - target="_blank", + target="_blank", + rel="noopener noreferrer", ), - dmc.Anchor( + dmc.Anchor( "Documentation", href="https://center-for-the-built-environment.gitbook.io/clima/", underline=True, c="white", - target="_blank", + target="_blank", + rel="noopener noreferrer", ), - dmc.Anchor( + dmc.Anchor( "License", href="https://center-for-the-built-environment.gitbook.io/clima/#license", underline=True, c="white", - target="_blank", + target="_blank", + rel="noopener noreferrer", ),Also applies to: 89-95, 97-103, 103-110, 110-116, 117-123
pages/select.py (2)
144-167: Remove hard-coded prop_id strings; bind to ElementIds to avoid desync.Comparing ctx.triggered against literal IDs undermines the centralization effort and will break if IDs change.
Apply:
- if ctx.triggered[0][ColNames.PROP_ID] == "modal-yes-button.n_clicks": + triggered = ctx.triggered[0].get(ColNames.PROP_ID, "") + if triggered == f"{ElementIds.MODAL_YES_BUTTON}.n_clicks": @@ - elif ( - ctx.triggered[0][ColNames.PROP_ID] == "upload-data.contents" - and list_of_contents is not None - ): + elif triggered == f"{ElementIds.UPLOAD_DATA}.contents" and list_of_contents is not None:
189-205: Fix swapped error messages.“Not EPW extension” should return wrong_extension; “parsing failure” should return invalid_format.
Apply:
- else: - return ( - None, - None, - True, - messages_alert["invalid_format"], - "warning", - ) - except (ValueError, IndexError, KeyError) as e: + else: + return ( + None, + None, + True, + messages_alert["wrong_extension"], + "warning", + ) + except (ValueError, IndexError, KeyError) as e: print(f"Error parsing EPW file: {e}") return ( None, None, True, - messages_alert["wrong_extension"], + messages_alert["invalid_format"], "warning", )Also applies to: 197-204
pages/natural_ventilation.py (4)
345-355: Fix wrong emptiness check and user-facing typo
- The emptiness check uses MONTH, but rows are nulled on var (DBT). The alert may never trigger. Check var instead.
- Typo: “thedew-point” → “the dew-point”.
- if df.dropna(subset=[ColNames.MONTH]).shape[0] == 0: + if df[var].dropna().shape[0] == 0: return ( dbc.Alert( - "Natural ventilation is not available in this location under these" + "Natural ventilation is not available in this location under these" " conditions. Please either select a different outdoor dry-bulb air" " temperature range, change the month and hour filter, or increase" - " thedew-point temperature.", + " the dew-point temperature.", color="danger", style={"text-align": "center", "marginTop": "2rem"}, ), )
421-426: margin dict.update() bug: returns Nonedict.update() returns None, so layout margin becomes None.
- margin=tight_margins.copy().update({"t": 55}), + margin={**tight_margins, "t": 55},
541-541: Guard against divide-by-zero when normalizingIf a month has zero hours after filtering, this produces inf/NaN.
- per_time_nv_allowed = np.round(100 * (n_hours_nv_allowed / tot_month_hours)) + with np.errstate(divide="ignore", invalid="ignore"): + per_time_nv_allowed = np.round( + 100 + * np.divide( + n_hours_nv_allowed, + tot_month_hours, + out=np.zeros_like(n_hours_nv_allowed, dtype=float), + where=tot_month_hours != 0, + ) + )
598-604: Same margin bug in bar chartApply the same fix as in nv_heatmap.
- margin=tight_margins.copy().update({"t": 55}), + margin={**tight_margins, "t": 55},pages/summary.py (2)
247-261: Add timeout and exception handling for climate APIPrevent server hangs and handle failures gracefully.
- r = requests.get( - f"http://climateapi.scottpinkelman.com/api/v1/location/{meta[ColNames.LAT]}/{meta[ColNames.LON]}" - ) + try: + r = requests.get( + f"http://climateapi.scottpinkelman.com/api/v1/location/{meta[ColNames.LAT]}/{meta[ColNames.LON]}", + timeout=5, + ) + except requests.RequestException: + r = NoneAnd guard on r is not None before status_code.
338-345: Trigger detection should not use ColNames.PROP_IDUse Dash’s prop_id directly and the ElementIds constant for the button id.
- ctx = dash.callback_context - - if ( - ctx.triggered[0][ColNames.PROP_ID] == "submit-set-points.n_clicks_timestamp" - or n_clicks is None - ): + ctx = dash.callback_context + prop_id = (ctx.triggered[0]["prop_id"] if ctx.triggered else "") + if prop_id == f"{ElementIds.SUBMIT_SET_POINTS}.n_clicks_timestamp" or n_clicks is None:
♻️ Duplicate comments (7)
pages/lib/extract_df.py (1)
168-168: Bug: FAKE_YEAR assigns a literal, not the column values.This sets the cell values to the string "year" instead of copying the year column. Move after dtype cast or copy directly now.
Apply one of the following diffs:
-epw_df[ColNames.FAKE_YEAR] = ColNames.YEAR +epw_df[ColNames.FAKE_YEAR] = epw_df[ColNames.YEAR]or (ensuring int, placed after the int cast block):
-epw_df[ColNames.FAKE_YEAR] = ColNames.YEAR +# after casting YEAR/MONTH/DAY/HOUR to int +epw_df[ColNames.FAKE_YEAR] = epw_df[ColNames.YEAR]pages/psy-chart.py (2)
78-85: Trailing-space fixes look good.The prior whitespace issues on ElementIds.PSY_VAR_DROPDOWN and State refs are resolved.
Also applies to: 95-99, 104-116, 174-182, 240-259
391-394: Bug: hovertemplate repeats name; unit missing (was flagged earlier).Replace the second DBT name with the DBT unit so hover reads “Name: value unit”.
Apply:
- hovertemplate=mapping_dictionary[ColNames.DBT][ColNames.NAME] - + ": %{x:.2f}" - + mapping_dictionary[ColNames.DBT][ColNames.NAME], + hovertemplate=mapping_dictionary[ColNames.DBT][ColNames.NAME] + + ": %{x:.2f}" + + mapping_dictionary[ColNames.DBT][si_ip][ColNames.UNIT],pages/t_rh.py (1)
55-57: Normalize title casing to match others.Use “Yearly chart” (lowercase “chart”) to be consistent with “Daily chart” and “Heatmap chart.”
Apply:
- text="Yearly Chart", + text="Yearly chart",pages/natural_ventilation.py (1)
515-519: Replace string "nv_allowed" with constant (repeat)The filter_df_by_month_and_hour call still passes the string literal. Use ColNames.NV_ALLOWED.
- df = filter_df_by_month_and_hour( - df, time_filter, month, hour, invert_month, invert_hour, "nv_allowed" - ) + df = filter_df_by_month_and_hour( + df, time_filter, month, hour, invert_month, invert_hour, ColNames.NV_ALLOWED + )pages/wind.py (2)
423-435: Wind direction callback should react to DF updates (repeat)Add DF store timestamp as an Input and adjust signature.
@callback( - Output(ElementIds.WIND_DIRECTION, "children"), + Output(ElementIds.WIND_DIRECTION, "children"), # General - [ - Input(ElementIds.ID_WIND_GLOBAL_LOCAL_RADIO_INPUT, "value"), - ], + [ + Input(ElementIds.ID_WIND_DF_STORE, "modified_timestamp"), + Input(ElementIds.ID_WIND_GLOBAL_LOCAL_RADIO_INPUT, "value"), + ], [ State(ElementIds.ID_WIND_DF_STORE, "data"), State(ElementIds.ID_WIND_META_STORE, "data"), State(ElementIds.ID_WIND_SI_IP_UNIT_STORE, "data"), ], ) -def update_tab_wind_direction(global_local, df, meta, si_ip): +def update_tab_wind_direction(_, global_local, df, meta, si_ip):
660-665: Noon selection uses morning time range (repeat)Use noon_times for noon_df.
- noon_df = df.loc[ - (df[ColNames.HOUR] >= morning_times[0]) & (df[ColNames.HOUR] <= morning_times[1]) - ] + noon_df = df.loc[ + (df[ColNames.HOUR] >= noon_times[0]) & (df[ColNames.HOUR] <= noon_times[1]) + ]
🧹 Nitpick comments (28)
pages/lib/extract_df.py (8)
182-193: Simplify and harden DOY computation.Current approach relies on groupby order; use the timestamp directly for determinism and fewer ops.
Apply:
- df_doy = ( - epw_df.groupby([ColNames.MONTH, ColNames.DAY])[ColNames.HOUR] - .count() - .reset_index() - ) - df_doy[ColNames.DOY] = df_doy.index + 1 - epw_df = pd.merge( - epw_df, - df_doy[[ColNames.MONTH, ColNames.DAY, ColNames.DOY]], - on=[ColNames.MONTH, ColNames.DAY], - how="left", - ) + epw_df[ColNames.DOY] = epw_df[ColNames.TIMES].dt.dayofyear
228-234: Prefer bracket access over attribute access for columns.Use epw_df[ColNames.TIMES] consistently to avoid attr/column name collisions and improve rename safety.
-epw_df.set_index( - ColNames.TIMES, drop=False, append=False, inplace=True, verify_integrity=False -) +m_times = ColNames.TIMES +epw_df.set_index(m_times, drop=False, append=False, inplace=True, verify_integrity=False)-mrt_df = mrt_df.set_index(epw_df.times) +mrt_df = mrt_df.set_index(epw_df[ColNames.TIMES])Also applies to: 268-269
243-254: Avoid 8760 magic number; size from data.Derive length from epw_df to handle leap years or non-standard files.
-sol_altitude = epw_df[ColNames.ELEVATION].mask(epw_df[ColNames.ELEVATION] <= 0, 0) -sharp = [45] * 8760 +sol_altitude = epw_df[ColNames.ELEVATION].mask(epw_df[ColNames.ELEVATION] <= 0, 0) +n = len(epw_df) +sharp = [45] * n ... -sol_transmittance = [1] * 8760 # CHECK VALUE -f_svv = [1] * 8760 # CHECK VALUE -f_bes = [1] * 8760 # CHECK VALUE -asw = [0.7] * 8760 # CHECK VALUE +sol_transmittance = [1] * n # CHECK VALUE +f_svv = [1] * n # CHECK VALUE +f_bes = [1] * n # CHECK VALUE +asw = [0.7] * n # CHECK VALUE ... -posture = ["standing"] * 8760 -floor_reflectance = [0.6] * 8760 # EXPOSE AS A VARIABLE? +posture = ["standing"] * n +floor_reflectance = [0.6] * n # EXPOSE AS A VARIABLE?
272-279: WIND_SPEED_UTCI clamping is fine; set zero-wind column directly.The mask with >= 0 sets almost all rows to 0.5 anyway; assign constant to be explicit (and include NaNs if desired using fillna).
Choose one:
-epw_df[ColNames.WIND_SPEED_UTCI_0] = epw_df[ColNames.WIND_SPEED_UTCI].mask( - epw_df[ColNames.WIND_SPEED_UTCI] >= 0, 0.5 -) +epw_df[ColNames.WIND_SPEED_UTCI_0] = 0.5or preserve NaNs:
-epw_df[ColNames.WIND_SPEED_UTCI_0] = epw_df[ColNames.WIND_SPEED_UTCI].mask( - epw_df[ColNames.WIND_SPEED_UTCI] >= 0, 0.5 -) +epw_df[ColNames.WIND_SPEED_UTCI_0] = epw_df[ColNames.WIND_SPEED_UTCI].where( + epw_df[ColNames.WIND_SPEED_UTCI].isna(), 0.5 +)Also applies to: 280-282
324-328: psy_ta_rh vectorization may be slow.If performance matters, consider batching without np.vectorize (if psy supports arrays) or using DataFrame.apply on smaller chunks. Keep as-is if not a hot path.
330-338: Use ColNames when querying DOY and ensure symbol consistency.Minor style/consistency: prefer bracket access.
-for day in epw_df.DOY.unique(): +for day in epw_df[ColNames.DOY].unique():
410-415: Avoid double json.loads and simplify lookup.Minor efficiency/readability tweak.
- mapping_dict = json.loads(mapping_json) - for key in json.loads(mapping_json): - if ColNames.CONVERSION_FUNCTION in mapping_dict[key]: - conversion_function_name = mapping_dict[key][ColNames.CONVERSION_FUNCTION] + mapping_dict = json.loads(mapping_json) + for key in mapping_dict: + conversion_function_name = mapping_dict[key].get(ColNames.CONVERSION_FUNCTION)Also applies to: 418-420
116-146: Prefer ColNames for column creation to fully eliminate literals.If ColNames defines these, initialize with constants for consistency across the module.
- col_names = [ - "year","month","day","hour","DBT","DPT","RH","p_atm","extr_hor_rad", - "hor_ir_rad","glob_hor_rad","dir_nor_rad","dif_hor_rad","glob_hor_ill", - "dir_nor_ill","dif_hor_ill","Zlumi","wind_dir","wind_speed","tot_sky_cover", - "Oskycover","Vis","Cheight","PWobs","PWcodes","Pwater","AsolOptD","SnowD","DaySSnow", - ] + col_names = [ + ColNames.YEAR, ColNames.MONTH, ColNames.DAY, ColNames.HOUR, + ColNames.DBT, ColNames.DPT, ColNames.RH, ColNames.P_ATM, + ColNames.EXTR_HOR_RAD, ColNames.HOR_IR_RAD, ColNames.GLOB_HOR_RAD, + ColNames.DIR_NOR_RAD, ColNames.DIF_HOR_RAD, ColNames.GLOB_HOR_ILL, + ColNames.DIR_NOR_ILL, ColNames.DIF_HOR_ILL, ColNames.ZLUMI, + ColNames.WIND_DIR, ColNames.WIND_SPEED, ColNames.TOT_SKY_COVER, + ColNames.OSKY_COVER, ColNames.VIS, ColNames.CHEIGHT, + ColNames.PWOBS, ColNames.PWCODES, ColNames.PWATER, + ColNames.ASOL_OPTD, ColNames.SNOWD, ColNames.DAYSSNOW, + ]pages/lib/global_id_buttons.py (4)
9-9: Rename and normalize PSYCHROMETRIC_CHART_CHART.Duplicate “_CHART” in the name and TitleCase in the value are inconsistent with the rest. Prefer a single “_LABEL” suffix and kebab-case value.
- PSYCHROMETRIC_CHART_CHART = "Psychrometric-Chart-chart" + PSYCHROMETRIC_CHART_LABEL = "psychrometric-chart-label"
5-5: Align naming with ElementIds.TABLE_TMP_HUM.“TMP_RH” vs “TMP_HUM” is inconsistent and easy to misuse.
- TABLE_TMP_RH = "table-tmp-rh" + TABLE_TMP_HUM = "table-tmp-hum"
1-29: Unify constants strategy (Enum vs plain class).Elsewhere you use a str Enum (ElementIds); consider the same here to improve type safety and introspection, or merge these into ElementIds under a “labels” section to avoid two sources of truth.
3-4: Use consistent kebab-case for NV_NORMALIZE
Change the constant’s value inpages/lib/global_id_buttons.pyfrom"nv_normalize"to"nv-normalize". No string literals or other references to"nv_normalize"were found—updating the definition will propagate automatically.pages/lib/global_element_ids.py (3)
62-63: Clarify intentional aliasing.MONTH_HOUR_FILTER and ID_OUTDOOR_MONTH_HOUR_FILTER share the same value. Aliases are fine, but add a brief comment/docstring explaining the convention to reduce confusion when iterating the Enum (aliases are skipped).
class ElementIds(str, Enum): + """Centralized element IDs. + Notes: + - Several members intentionally alias the same underlying ID (e.g., df-store) for semantic clarity per page. + - When iterating this Enum, Python skips aliases; access members by name. + """
5-7: Add missing ID for title_with_tooltip buttons.Reviewer requested an ID for the id_button used in title_with_tooltip; add it here to standardize usage.
class ElementIds(str, Enum): @@ OUTDOOR_COMFORT_OUTPUT = "outdoor-comfort-output" + # Title-with-tooltip button (used across pages) + ID_TITLE_WITH_TOOLTIP_BUTTON = "id_button"Also applies to: 205-209
14-14: Optional: namespace the generic “dropdown” ID
No direct"dropdown"literals were found outside this alias—safe to refactor. Consider renaming the constant at pages/lib/global_element_ids.py:14 to something like"t-rh-dropdown"and updating all references to minimize collision risk.main.py (1)
17-21: Optional: centralize dcc.Location('url').Consider adding ElementIds.URL = "url" and using it here to avoid a stray literal.
- dcc.Location(id="url", refresh=False), # connected to callback below + dcc.Location(id=ElementIds.URL, refresh=False), # connected to callback belowpages/lib/layout.py (1)
35-39: Unify single source for interval/alert IDs.Both ID_LAYOUT_* and ID_MAIN_* map to the same underlying values. Consider keeping just one canonical pair to reduce cognitive load.
Also applies to: 205-209
pages/lib/global_tab_names.py (1)
1-48: Optional: Standardize TabNames casing or convert to StrEnum
- Normalize all
TabNamesvalues to snake_case for consistency- Convert
TabNamesto aStrEnumto enforce safer reuse- Update
generate_chart_namecalls in pages/t_rh.py, pages/summary.py, and pages/wind.py to match the new valuespages/sun.py (1)
40-54: Minor: simplify sc_dropdown_names construction.Avoid multiple update/pop calls to reduce churn and risk of missing keys.
Apply:
sc_dropdown_names = { - "None": "None", - "Frequency": "Frequency", -} -sc_dropdown_names.update(deepcopy(dropdown_names)) -sc_dropdown_names.update(deepcopy(sun_cloud_tab_dropdown_names)) -sc_dropdown_names.update(deepcopy(sun_cloud_tab_explore_dropdown_names)) -# Remove the keys from the dictionary -sc_dropdown_names.pop("Vapor partial pressure", None) -sc_dropdown_names.pop("Absolute humidity", None) -sc_dropdown_names.pop("UTCI: Sun & Wind : categories", None) -sc_dropdown_names.pop("UTCI: no Sun & Wind : categories", None) -sc_dropdown_names.pop("UTCI: Sun & no Wind : categories", None) -sc_dropdown_names.pop("UTCI: no Sun & no Wind : categories", None) + "None": "None", + "Frequency": "Frequency", + **deepcopy(dropdown_names), + **deepcopy(sun_cloud_tab_dropdown_names), + **deepcopy(sun_cloud_tab_explore_dropdown_names), +} +for k in ( + "Vapor partial pressure", + "Absolute humidity", + "UTCI: Sun & Wind : categories", + "UTCI: no Sun & Wind : categories", + "UTCI: Sun & no Wind : categories", + "UTCI: no Sun & no Wind : categories", +): + sc_dropdown_names.pop(k, None)pages/psy-chart.py (1)
456-474: Prefer constants over raw strings for customdata keys."h" and "t_dp" are still string literals; for consistency with the refactor, consider adding ColNames for these if available and using them everywhere (mapping_dictionary and df access).
pages/t_rh.py (1)
195-206: Single-output callback declared as list of Outputs.Style-only: declare a single Output directly for readability.
Example:
-@callback( - [Output(ElementIds.HEATMAP, "children")], +@callback( + Output(ElementIds.HEATMAP, "children"),pages/select.py (2)
356-362: Use a column name for hover_name, not a Series.Pass the column key string so Plotly can handle labels consistently.
Apply:
- fig2 = px.scatter_mapbox( + fig2 = px.scatter_mapbox( df_one_building, lat="lat", lon="lon", - hover_name=df_one_building[ColNames.NAME], + hover_name=ColNames.NAME,
223-234: map_json is computed then unused.Drop dead code or persist it if needed.
Apply:
- map_json = json.dumps(mapping_dictionary) - if si_ip_input == UnitSystem.IP: - map_json = convert_data(df, map_json) + # If mapping conversion is needed for client-side use, persist it to a store; otherwise remove.pages/outdoor.py (1)
350-401: Avoid suffix concatenation in multiple places.var + "_categories" appears in two callbacks; consider a helper (e.g., to_categories(var)) or a ColNames.MAKE_CATEGORY(var) API to prevent typos.
pages/natural_ventilation.py (1)
535-539: Be explicit with subset listAvoid passing a bare string to subset to prevent ambiguity.
- n_hours_nv_allowed = ( - df.dropna(subset=ColNames.NV_ALLOWED) + n_hours_nv_allowed = ( + df.dropna(subset=[ColNames.NV_ALLOWED]) .groupby(df[ColNames.UTC_TIME].dt.month)[ColNames.NV_ALLOWED] .sum() .values )pages/explorer.py (2)
72-75: Default dropdown value should use constantUse ColNames.DBT for consistency.
- value="DBT", + value=ColNames.DBT,
452-458: Use ColNames for color-by defaultAlign with constants.
- value="glob_hor_rad", + value=ColNames.GLOB_HOR_RAD,pages/summary.py (1)
534-542: CSV downloads: exclude the indexAvoid including the implicit index column.
- df.to_csv, + df.to_csv, f"df_{meta[ColNames.CITY]}_{meta[ColNames.COUNTRY]}_Clima_SIunit.csv", + index=False, )- df.to_csv, + df.to_csv, f"df_{meta[ColNames.CITY]}_{meta[ColNames.COUNTRY]}_Clima_IPunit.csv", + index=False, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
main.py(2 hunks)pages/explorer.py(43 hunks)pages/lib/charts_summary.py(1 hunks)pages/lib/charts_sun.py(16 hunks)pages/lib/extract_df.py(8 hunks)pages/lib/global_column_names.py(1 hunks)pages/lib/global_element_ids.py(1 hunks)pages/lib/global_id_buttons.py(1 hunks)pages/lib/global_scheme.py(2 hunks)pages/lib/global_tab_names.py(1 hunks)pages/lib/layout.py(8 hunks)pages/lib/template_graphs.py(25 hunks)pages/lib/utils.py(7 hunks)pages/natural_ventilation.py(25 hunks)pages/outdoor.py(18 hunks)pages/psy-chart.py(21 hunks)pages/select.py(15 hunks)pages/summary.py(17 hunks)pages/sun.py(12 hunks)pages/t_rh.py(4 hunks)pages/wind.py(28 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- pages/lib/charts_summary.py
- pages/lib/global_scheme.py
- pages/lib/utils.py
- pages/lib/template_graphs.py
- pages/lib/charts_sun.py
- pages/lib/global_column_names.py
🔇 Additional comments (28)
pages/lib/extract_df.py (8)
22-22: Good move centralizing column names.Importing ColNames here aligns the file with the new constants strategy.
71-73: Consistent use of ColNames.PERIOD.Looks good; verify that ColNames.PERIOD equals "period" to match the dict keys initialized above.
Also applies to: 99-101
156-166: EnergyPlus years → PERIOD computed via ColNames.YEAR.LGTM; logic preserved while removing literals.
172-174: Month names mapping via ColNames.Looks correct and keeps mapping centralized.
177-179: Type casting via ColNames.Good consistency; no issues spotted.
283-306: UTCI calculations migrated to ColNames.Reads clean and consistent.
310-321: Categories refactor looks good.Consistent ColNames usage; bins/labels unchanged.
358-371: Adaptive outputs mapped via ColNames.Nicely standardized; no issues.
pages/lib/global_element_ids.py (1)
68-71: Rename SDATA_FILTER → SEC1_DATA_FILTER_INPUT and update its literal- SDATA_FILTER = "data-filter" + SEC1_DATA_FILTER_INPUT = "sec1-data-filter-input"No other occurrences of
SDATA_FILTERorSEC1_DATA_FILTER_INPUTwere found; verify withrg -nP '\bSDATA_FILTER\b'and update any remaining references.
main.py (1)
26-29: LGTM: callback wired to ElementIds.Switching to ElementIds keeps IDs centralized and consistent.
pages/lib/layout.py (1)
239-266: ColNames usage is correct – no changes needed. ColNames.NAME and ColNames.PATH are defined in pages/lib/global_column_names.py as "name" and "path" and are intentionally reused for UI dict keys.pages/sun.py (2)
61-67: Good: centralized id_button via IdButtons.Using IdButtons for title_with_link resolves the reviewer’s “missing ID entries” note and avoids ad-hoc IDs.
78-86: IDs/ColNames refactor looks consistent.Inputs/Outputs/States and chart labels all reference ElementIds/ColNames/TabNames; this aligns with the PR goal and fixes prior string literals in sun.py (e.g., TOT_SKY_COVER).
Also applies to: 97-103, 106-111, 152-159, 164-168, 171-174, 187-191, 198-202, 205-218, 239-247, 250-281, 284-305, 307-326
pages/t_rh.py (1)
262-268: Remove redundant dd_value validation—dropdown_names only outputs valid df columns. dd_value is sourced from dropdown_names built from variables_dropdown (which only includes “DBT” and “RH”), matching actual DataFrame columns, so no extra check is needed.Likely an incorrect or invalid review comment.
pages/select.py (1)
248-283: Good: banner subtitle uses ColNames for dynamic location.Centralizing keys avoids brittle string indexing.
pages/outdoor.py (2)
221-263: Nice: descriptive string returned from data analysis.Robust to missing columns and types; exception handling is appropriate.
296-312: TabNames/units wiring is consistent.Config generation aligns with the refactor; LGTM.
Also applies to: 395-401, 438-440
pages/natural_ventilation.py (3)
21-24: Consistent constants importGood move centralizing names via ColNames, ElementIds, IdButtons, and TabNames.
394-418: LGTM: axes/data wiringUsing ColNames for axis/customdata is correct and consistent.
522-525: tot_month_hours computationLooks correct post time-filter; keep.
pages/explorer.py (4)
110-117: Docs/title wiringIdButtons usage looks correct; nice.
237-241: Good: default to ColNames.RHConsistent with the constants strategy.
965-975: Table summary uses constants correctlyMONTH, HOUR, MONTH_NAMES constants applied properly.
884-886: filter_df_by_month_and_hour supports iterable column selectors: Pandas.locaccepts both single labels and array-like column selectors, so passingdf.columnscorrectly masks all columns. Performance is typical for a bulk.locassignment; if profiling shows a bottleneck, narrowvarto only the needed subset of columns.pages/wind.py (1)
57-65: Hour slider range differs from other tabsWind uses 1–24 while others (e.g., NV) use 0–24. Confirm the intended domain across the app to avoid off-by-one filtering mismatches.
pages/summary.py (3)
266-282: Solar radiation summary updateUnit derivation with “k” prefix and DBT stats look correct.
420-423: Chart config/idGood usage of TabNames and ElementIds for the degree-days chart.
520-563: Download filenames use constants correctlyCity/country via ColNames is consistent.
pages/natural_ventilation.py
Outdated
| var = "DBT" | ||
| filter_var = "DPT" | ||
|
|
||
| var_unit = mapping_dictionary[var][si_ip]["unit"] | ||
| filter_unit = mapping_dictionary[filter_var][si_ip]["unit"] | ||
| var_unit = mapping_dictionary[var][si_ip][ColNames.UNIT] | ||
| filter_unit = mapping_dictionary[filter_var][si_ip][ColNames.UNIT] | ||
|
|
||
| var_name = mapping_dictionary[var]["name"] | ||
| var_name = mapping_dictionary[var][ColNames.NAME] | ||
|
|
||
| filter_name = mapping_dictionary[filter_var]["name"] | ||
| filter_name = mapping_dictionary[filter_var][ColNames.NAME] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align var/filter_var to constants
Keep consistency with nv_heatmap by using ColNames.
- var = "DBT"
- filter_var = "DPT"
+ var = ColNames.DBT
+ filter_var = ColNames.DPT📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var = "DBT" | |
| filter_var = "DPT" | |
| var_unit = mapping_dictionary[var][si_ip]["unit"] | |
| filter_unit = mapping_dictionary[filter_var][si_ip]["unit"] | |
| var_unit = mapping_dictionary[var][si_ip][ColNames.UNIT] | |
| filter_unit = mapping_dictionary[filter_var][si_ip][ColNames.UNIT] | |
| var_name = mapping_dictionary[var]["name"] | |
| var_name = mapping_dictionary[var][ColNames.NAME] | |
| filter_name = mapping_dictionary[filter_var]["name"] | |
| filter_name = mapping_dictionary[filter_var][ColNames.NAME] | |
| var = ColNames.DBT | |
| filter_var = ColNames.DPT | |
| var_unit = mapping_dictionary[var][si_ip][ColNames.UNIT] | |
| filter_unit = mapping_dictionary[filter_var][si_ip][ColNames.UNIT] | |
| var_name = mapping_dictionary[var][ColNames.NAME] | |
| filter_name = mapping_dictionary[filter_var][ColNames.NAME] |
🤖 Prompt for AI Agents
In pages/natural_ventilation.py around lines 503–512, replace the hard-coded
string keys "DBT" and "DPT" with the ColNames constants used elsewhere (as done
in nv_heatmap) — e.g. set var = ColNames.DBT and filter_var = ColNames.DPT (or
the exact DBT/DPT constant names used in ColNames in this codebase), then use
those constants for the mapping_dictionary lookups for UNIT and NAME so all
accesses remain mapping_dictionary[var][si_ip][ColNames.UNIT] and
mapping_dictionary[var][ColNames.NAME], keeping consistency with nv_heatmap.
| children=title_with_link( | ||
| text="UTCI heatmap chart", | ||
| id_button="utci-charts-label", | ||
| id_button=IdButtons.UTCI_CHARTS_LABEL, | ||
| doc_link=DocLinks.UTCI_CHART, | ||
| ) | ||
| ), |
There was a problem hiding this comment.
Duplicate id_button for two different titles → DOM ID collision.
Both titles use IdButtons.UTCI_CHARTS_LABEL; this creates duplicate IDs on the same page and can break callbacks/tooltip bindings.
Apply:
- children=title_with_link(
- text="UTCI heatmap chart",
- id_button=IdButtons.UTCI_CHARTS_LABEL,
+ children=title_with_link(
+ text="UTCI heatmap chart",
+ id_button=IdButtons.UTCI_HEATMAP_CHART_LABEL,
@@
- children=title_with_link(
- text="UTCI thermal stress chart",
- id_button=IdButtons.UTCI_CHARTS_LABEL,
+ children=title_with_link(
+ text="UTCI thermal stress chart",
+ id_button=IdButtons.UTCI_THERMAL_STRESS_CHART_LABEL,Add these in pages/lib/global_id_buttons.py:
# New IDs to avoid collisions
UTCI_HEATMAP_CHART_LABEL = "id-button-utci-heatmap-chart"
UTCI_THERMAL_STRESS_CHART_LABEL = "id-button-utci-thermal-stress-chart"Also applies to: 162-167
🤖 Prompt for AI Agents
In pages/outdoor.py around lines 151-156 (and similarly lines 162-167) there is
a DOM ID collision because both title components use
IdButtons.UTCI_CHARTS_LABEL; replace those duplicates with two distinct IDs
(e.g., UTCI_HEATMAP_CHART_LABEL and UTCI_THERMAL_STRESS_CHART_LABEL) and update
the title_with_link calls accordingly; also add the corresponding constants to
pages/lib/global_id_buttons.py (define the two new string IDs) so the imports
resolve and avoid duplicate IDs on the page.
| id=ElementIds.GH_RAD_PROFILE_GRAPH, | ||
| config=generate_chart_name(TabNames.MAP, meta), | ||
| figure=world_map(meta), |
There was a problem hiding this comment.
Duplicate component id: GH_RAD_PROFILE_GRAPH used for world map
This id is also used for the solar radiation graph, causing duplicate IDs. Remove the id or use a dedicated one.
- id=ElementIds.GH_RAD_PROFILE_GRAPH,
+ # id intentionally omitted to avoid collision with the solar radiation graph📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| id=ElementIds.GH_RAD_PROFILE_GRAPH, | |
| config=generate_chart_name(TabNames.MAP, meta), | |
| figure=world_map(meta), | |
| # id intentionally omitted to avoid collision with the solar radiation graph | |
| config=generate_chart_name(TabNames.MAP, meta), | |
| figure=world_map(meta), |
🤖 Prompt for AI Agents
In pages/summary.py around lines 211 to 213 the component is assigned
id=ElementIds.GH_RAD_PROFILE_GRAPH which duplicates the solar radiation graph
id; change this to a unique id (either remove the id attribute entirely or use a
dedicated constant such as ElementIds.GH_WORLD_MAP_GRAPH), update the ElementIds
enum/consts if needed, and replace the id reference here so each component has a
distinct id.
| query = query_month + str(i) + " and DBT<=" + str(hdd_setpoint) | ||
| a = df.query(query)["DBT"].sub(hdd_setpoint) | ||
| a = df.query(query)[ColNames.DBT].sub(hdd_setpoint) | ||
| hdd = a.sum(axis=0, skipna=True) | ||
| hdd = hdd / 24 | ||
| hdd = int(hdd) | ||
| hdd_array.append(hdd) | ||
|
|
||
| # calculates CDD per month | ||
| query = query_month + str(i) + " and DBT>=" + str(cdd_setpoint) | ||
| a = df.query(query)["DBT"].sub(cdd_setpoint) | ||
| a = df.query(query)[ColNames.DBT].sub(cdd_setpoint) | ||
| cdd = a.sum(axis=0, skipna=True) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid string queries; use boolean indexing and constants
String-based DataFrame.query is brittle and mixes literals. Use ColNames and boolean masks.
Example for HDD loop (apply similarly to CDD):
- query = query_month + str(i) + " and DBT<=" + str(hdd_setpoint)
- a = df.query(query)[ColNames.DBT].sub(hdd_setpoint)
+ mask = (df[ColNames.MONTH] == i) & (df[ColNames.DBT] <= hdd_setpoint)
+ a = df.loc[mask, ColNames.DBT].sub(hdd_setpoint)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| query = query_month + str(i) + " and DBT<=" + str(hdd_setpoint) | |
| a = df.query(query)["DBT"].sub(hdd_setpoint) | |
| a = df.query(query)[ColNames.DBT].sub(hdd_setpoint) | |
| hdd = a.sum(axis=0, skipna=True) | |
| hdd = hdd / 24 | |
| hdd = int(hdd) | |
| hdd_array.append(hdd) | |
| # calculates CDD per month | |
| query = query_month + str(i) + " and DBT>=" + str(cdd_setpoint) | |
| a = df.query(query)["DBT"].sub(cdd_setpoint) | |
| a = df.query(query)[ColNames.DBT].sub(cdd_setpoint) | |
| cdd = a.sum(axis=0, skipna=True) | |
| # calculates HDD per month using boolean indexing | |
| mask = (df[ColNames.MONTH] == i) & (df[ColNames.DBT] <= hdd_setpoint) | |
| a = df.loc[mask, ColNames.DBT].sub(hdd_setpoint) | |
| hdd = a.sum(axis=0, skipna=True) | |
| hdd = hdd / 24 | |
| hdd = int(hdd) | |
| hdd_array.append(hdd) | |
| # calculates CDD per month (apply the same pattern here) | |
| query = query_month + str(i) + " and DBT>=" + str(cdd_setpoint) | |
| a = df.query(query)[ColNames.DBT].sub(cdd_setpoint) | |
| cdd = a.sum(axis=0, skipna=True) |
| text="Seasonal Wind Rose", | ||
| id_button="seasonal-rose-chart", | ||
| id_button=ElementIds.SEASONAL_WIND_ROSE_DOC, | ||
| doc_link=DocLinks.WIND_ROSE, | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Doc button should use IdButtons, not ElementIds
Keep id_button aligned with IdButtons.* like elsewhere.
- id_button=ElementIds.SEASONAL_WIND_ROSE_DOC,
+ id_button=IdButtons.SEASONAL_WIND_ROSE_DOC,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| text="Seasonal Wind Rose", | |
| id_button="seasonal-rose-chart", | |
| id_button=ElementIds.SEASONAL_WIND_ROSE_DOC, | |
| doc_link=DocLinks.WIND_ROSE, | |
| ), | |
| text="Seasonal Wind Rose", | |
| id_button=IdButtons.SEASONAL_WIND_ROSE_DOC, | |
| doc_link=DocLinks.WIND_ROSE, | |
| ), |
🤖 Prompt for AI Agents
In pages/wind.py around lines 79 to 82, the doc button uses
ElementIds.SEASONAL_WIND_ROSE_DOC but should use IdButtons.* to match other
usages; replace id_button=ElementIds.SEASONAL_WIND_ROSE_DOC with
id_button=IdButtons.SEASONAL_WIND_ROSE_DOC (preserve the same
alignment/indentation as surrounding args).
…e_var to constants.
Update instances of "wind_speed" to ColNames.WIND_SPEED in multiple files to improve code consistency and maintainability.
Update the label for the yearly chart to use consistent capitalization for improved readability and consistency in the UI.
FedericoTartarini
merged commit 795199f
into
CenterForTheBuiltEnvironment:development
6 participants
Code Sanitisation #159
Contributing.md
We are not sure if these changes meet your needs. If there are any parts that need improvement, please comment and we will check and make changes in time.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor